fix: Consider an empty string header value to be present #832
Conversation
| } | ||
|
|
||
| return !value.isEmpty | ||
| headers.index(of: name) != nil |
There was a problem hiding this comment.
The previous definition of exists was wrong because it considered a header with an empty string for its value to "not exist". Turns out a header with empty string value is valid, so it should definitely "exist".
I suspect that the reason for this logic is that the previous developers changed the definition of "exists" to suit the behavior demanded by empty-header protocol tests, which is wrong and is being fixed by Smithy team.
| .testTarget( | ||
| name: "SmithyHTTPAPITests", | ||
| dependencies: ["SmithyHTTPAPI"] | ||
| ), |
There was a problem hiding this comment.
Added a new test target & moved the Headers tests into it, since Headers is in the SmithyHTTPAPI module.
| func test_exists_trueIfHeaderValueIsEmptyString() { | ||
| let subject = Headers(["a": ""]) | ||
| XCTAssertTrue(subject.exists(name: "a")) | ||
| } |
There was a problem hiding this comment.
These new tests check the expected behavior for the exists() method on Headers.
|
This PR is failing the downstream CI builds because the This PR should merge when this PR ships in a new version of Smithy: |
|
Closing this PR because this change is being made in #843 (update to Smithy 1.52) instead. |
1 participant
Issue #
Description of changes
An empty string is an acceptable value for a HTTP header, and SDKs should send an empty string value for a header when one is passed.
Swift implements this behavior correctly, but incorrectly validates "forbidden headers" in a protocol test by considering a HTTP header to "not exist" when its value is an empty string.
This PR fixes the
exists()method on theHeaderstype so that it returnstruerather thanfalsewhen the header being checked has an empty string as its value.Note that the current
NullAndEmptyHeadersprotocol tests (in RestJSON & RestXML) for this behavior are incorrect, and this PR will cause the current version of these tests to fail. These tests are being corrected in a future version of Smithy and this PR should be merged only when that Smithy version is adopted.Also:
SmithyHTTPAPITeststest target and move theHeaderstests into it fromClientRuntimeTests.Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.